IMO this is not very good idea. Changes a bunch of comments, variable names, mostly in deps. At the very least you should exclude deps

Agreed. If you leave out the changes in deps/, this should be OK.

I agree we should exclude deps/, but I think we should also more generally exclude any dependencies, no matter where they are located. That includes deps/ as well as various third-party dependencies in tools/ (e.g. tools/eslint and tools/gyp) for example.

@cjihrig @vkurchatkin Sure, will update

@mscdex @cjihrig @vkurchatkin Updated. Think I got the right surface area here? I removed any node_modules as well. (Of course I'll rebase)

@bf4 I think the tools/eslint, tools/gyp, and tools/icu changes should be reverted as well as those changes are all to upstream files.

@mscdex Added another commit removing anything from tools for ease of review

A few nits and a question, otherwise the rest LGTM though.

@mscdex Thanks for your review. I'll wait on getting confirmation of how to handle the changelogs before making more changes, then I'll push changes and squash down to one commit.

@mscdex @thefourtheye should I wait for any more comments before updating?

@bf4 Feel free to update, we're just waiting on more input from other collaborators on the change in the one error message string.

@mscdex @cjihrig I've updated the PR to add [sic] for typos we're quoting from other sources, and moved the breaking changed into a code comment.

--- a/src/node_lttng_provider.h
+++ b/src/node_lttng_provider.h
@@ -61,7 +61,7 @@ void NODE_NET_STREAM_END(node_lttng_connection_t* conn,
 void NODE_GC_START(v8::GCType type,
                    v8::GCCallbackFlags flags,
                    v8::Isolate* isolate) {
-  const char* typeStr = "Unknown GC Type";
+  const char* typeStr = "Unkown GC Type"; // [sic]
   const char* flagsStr = "Unknown GC Flag";

 #define BUILD_IF(f) if (type == v8::GCType::f) { typeStr = #f; }
@@ -78,7 +78,7 @@ void NODE_GC_START(v8::GCType type,
 void NODE_GC_DONE(v8::GCType type,
                   v8::GCCallbackFlags flags,
                   v8::Isolate* isolate) {
-  const char* typeStr = "Unknown GC Type";
+  const char* typeStr = "Unkown GC Type"; // [sic]
   const char* flagsStr = "Unknown GC Flag";

@bf4 There's a typo in node.cc that you might want to include (s/enought/enough/).

@mscdex Pushed. would you like me to rebase and squash down to one commit?

@bf4 Nah, that can be done when landing. I plan on landing this tonight if nobody else beats me to it.

CI: https://ci.nodejs.org/job/node-test-pull-request/6581/

@jasnell If I'm reading that correctly, there's a conflict from an earlier commit in the PR? CONFLICT (modify/delete): deps/v8/src/base/atomicops_internals_arm_gcc.h deleted in HEAD and modified in Correct typos. Version Correct typos of deps/v8/src/base/atomicops_internals_arm_gcc.h left in tree. Would a rebase address that?

CI: https://ci.nodejs.org/job/node-test-pull-request/6620/

@bf4 It looks like the linter is complaining.

CI: https://ci.nodejs.org/job/node-test-pull-request/6626/

I'm finding it unusually difficult to find the actual failure in the CI logs.

I rebased off of master to adjust for the conflicting file that was moved 3c92200

@bf4 There weren't any failures in the last CI run. However, here is another run with your recent change: https://ci.nodejs.org/job/node-test-pull-request/6632/

@mscdex I don't see how those failure could be related to this PR

When I drill down

https://ci.nodejs.org/job/node-test-binary-windows/6806/RUN_SUBSET=1,VS_VERSION=vs2015,label=win2008r2/tapResults/ relates to a file lib/_debugger.js not in this diff

// parallel/test-debug-usage	

assert.js:81
  throw new assert.AssertionError({
  ^
AssertionError: 'Usage: node debug script.js\n' === 'Usage: node debug script.js\n       node debug <host>:<port>\n       node debug -p <pid>\n'
    at ChildProcess.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vs2015\label\win2008r2\test\parallel\test-debug-usage.js:20:10)
    at ChildProcess.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vs2015\label\win2008r2\test\common.js:452:15)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:194:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:208:12)

and the other https://ci.nodejs.org/job/node-test-commit-arm/8016/nodes=armv8-ubuntu1404/console relates to a failure to publish TAP results

ERROR: Step ‘Publish TAP Results’ failed: no workspace for node-test-commit-arm/nodes=armv8-ubuntu1404 #8016
Checking ^not ok
ERROR: Build step failed with exception
java.lang.NullPointerException
	at hudson.plugins.textfinder.TextFinderPublisher.findText(TextFinderPublisher.java:101)
	at hudson.plugins.textfinder.TextFinderPublisher.perform(TextFinderPublisher.java:74)
	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:779)
	at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:720)
	at hudson.model.Build$BuildExecution.post2(Build.java:185)
	at hudson.model.AbstractBuild$AbstractBuildExecution.post(AbstractBuild.java:665)
	at hudson.model.Run.execute(Run.java:1753)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:404)
Build step 'Jenkins Text Finder' marked build as failure
Notifying upstream projects of job completion
Finished: FAILURE

I do wonder if src/string-search.h should be included as I think it's coming from deps/v8/src/string-search.h but they're not the same

@bf4 You're correct, they're unrelated. The changes should be good to go now.

Lemme know if there's anything else for me to do.

Landed in 4897ae2. Thanks!

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land